Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

OpenZFS 7578 - Fix/improve some aspects of ZIL writing. #6191

Merged
merged 1 commit into from
Jun 9, 2017

Conversation

dinatale2
Copy link
Contributor

  • After some ZIL changes 6 years ago zil_slog_limit got partially broken
    due to zl_itx_list_sz not updated when async itx'es upgraded to sync.
    Actually because of other changes about that time zl_itx_list_sz is not
    really required to implement the functionality, so this patch removes
    some unneeded broken code and variables.
  • Original idea of zil_slog_limit was to reduce chance of SLOG abuse by
    single heavy logger, that increased latency for other (more latency critical)
    loggers, by pushing heavy log out into the main pool instead of SLOG. Beside
    huge latency increase for heavy writers, this implementation caused double
    write of all data, since the log records were explicitly prepared for SLOG.
    Since we now have I/O scheduler, I've found it can be much more efficient
    to reduce priority of heavy logger SLOG writes from ZIO_PRIORITY_SYNC_WRITE
    to ZIO_PRIORITY_ASYNC_WRITE, while still leave them on SLOG.
  • Existing ZIL implementation had problem with space efficiency when it
    has to write large chunks of data into log blocks of limited size. In some
    cases efficiency stopped to almost as low as 50%. In case of ZIL stored on
    spinning rust, that also reduced log write speed in half, since head had to
    uselessly fly over allocated but not written areas. This change improves
    the situation by offloading problematic operations from z*_log_write() to
    zil_lwb_commit(), which knows real situation of log blocks allocation and
    can split large requests into pieces much more efficiently. Also as side
    effect it removes one of two data copy operations done by ZIL code WR_COPIED
    case.
  • While there, untangle and unify code of z*_log_write() functions.
    Also zfs_log_write() alike to zvol_log_write() can now handle writes crossing
    block boundary, that may also improve efficiency if ZPL is made to do that.

Sponsored by: iXsystems, Inc.

Authored by: Alexander Motin [email protected]
Reviewed by: Matthew Ahrens [email protected]
Reviewed by: Prakash Surya [email protected]
Reviewed by: Andriy Gapon [email protected]
Reviewed by: Steven Hartland [email protected]
Reviewed by: Brad Lewis [email protected]
Reviewed by: Richard Elling [email protected]
Approved by: Robert Mustacchi [email protected]
Ported-by: Giuseppe Di Natale [email protected]

OpenZFS-issue: https://www.illumos.org/issues/7578
OpenZFS-commit: openzfs/openzfs@aeb13ac

Checklist:

  • My code follows the ZFS on Linux code style requirements.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.
  • All commit messages are properly formatted and contain Signed-off-by.
  • Change has been approved by a ZFS on Linux member.

@mention-bot
Copy link

@dinatale2, thanks for your PR! By analyzing the history of the files in this pull request, we identified @bprotopopov, @behlendorf and @tuxoko to be potential reviewers.

@behlendorf
Copy link
Contributor

@dinatale2 nice job reconciling the differences with upstream OpenZFS. I only added two small additional commits for fairly minor issues.

@behlendorf behlendorf requested a review from ryao June 6, 2017 18:35
@behlendorf
Copy link
Contributor

@ryao it would be great if you could review this OpenZFS patch which include changes to the ZIL.

Copy link
Contributor

@behlendorf behlendorf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The updated version of this LGTM and preserves the fastwrite logic. One possible addition which occurred to me is we could add an option to switch between the fastwrite metaslab allocator logic and the alternate OpenZFS logic. This would aid benchmarking.

 - After some ZIL changes 6 years ago zil_slog_limit got partially broken
due to zl_itx_list_sz not updated when async itx'es upgraded to sync.
Actually because of other changes about that time zl_itx_list_sz is not
really required to implement the functionality, so this patch removes
some unneeded broken code and variables.
 - Original idea of zil_slog_limit was to reduce chance of SLOG abuse by
single heavy logger, that increased latency for other (more latency critical)
loggers, by pushing heavy log out into the main pool instead of SLOG.  Beside
huge latency increase for heavy writers, this implementation caused double
write of all data, since the log records were explicitly prepared for SLOG.
Since we now have I/O scheduler, I've found it can be much more efficient
to reduce priority of heavy logger SLOG writes from ZIO_PRIORITY_SYNC_WRITE
to ZIO_PRIORITY_ASYNC_WRITE, while still leave them on SLOG.
 - Existing ZIL implementation had problem with space efficiency when it
has to write large chunks of data into log blocks of limited size.  In some
cases efficiency stopped to almost as low as 50%.  In case of ZIL stored on
spinning rust, that also reduced log write speed in half, since head had to
uselessly fly over allocated but not written areas.  This change improves
the situation by offloading problematic operations from z*_log_write() to
zil_lwb_commit(), which knows real situation of log blocks allocation and
can split large requests into pieces much more efficiently.  Also as side
effect it removes one of two data copy operations done by ZIL code WR_COPIED
case.
 - While there, untangle and unify code of z*_log_write() functions.
Also zfs_log_write() alike to zvol_log_write() can now handle writes crossing
block boundary, that may also improve efficiency if ZPL is made to do that.

Sponsored by:   iXsystems, Inc.

Authored by: Alexander Motin <[email protected]>
Reviewed by: Matthew Ahrens <[email protected]>
Reviewed by: Prakash Surya <[email protected]>
Reviewed by: Andriy Gapon <[email protected]>
Reviewed by: Steven Hartland <[email protected]>
Reviewed by: Brad Lewis <[email protected]>
Reviewed by: Richard Elling <[email protected]>
Approved by: Robert Mustacchi <[email protected]>
Ported-by: Giuseppe Di Natale <[email protected]>

OpenZFS-issue: https://www.illumos.org/issues/7578
OpenZFS-commit: openzfs/openzfs@aeb13ac
@sempervictus
Copy link
Contributor

This currently doesn't merge in for testing - 33c316d altered the wr_state piece in the by_dbuf changes.

Copy link
Contributor

@ryao ryao left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me. While I think that reducing the IO priority needs more scrutiny, I do not consider that to be a reason to block merging this. We can revisit that in the future if it is found to cause regressions.

@ryao
Copy link
Contributor

ryao commented Jun 9, 2017

@sempervictus That commit is not in HEAD. It is in #6058, which has been refreshed. If this is merged first, it will be simple to rebase on top of it.

@sempervictus
Copy link
Contributor

sempervictus commented Jun 9, 2017 via email

@behlendorf behlendorf merged commit 1b7c1e5 into openzfs:master Jun 9, 2017
@behlendorf
Copy link
Contributor

@ryao @sempervictus merged, you can rebase on top it when your ready.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants